-
Notifications
You must be signed in to change notification settings - Fork 1
Real blockchain transactions with Zaino/LWD backends and pexpect wallet integration with full M2 and zingolib integration #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Made changes requested @pacu thank you would await reviews |
|
Hi @Timi16 you have requested a re-review of this PR but I don't see any other changes. |
|
So i decided to change the faucet from Python to Rust so i can interact with Zingolub directly |
pacu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partial review.
not tested yet. there are some naming issues remaining. Also rust faucet should use librustzcash instead ofZebra RPC to validate addresses. Also left questions to the developer about method visibility on Axum handlers
pacu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've finished reviewing the code but I haven't tested it yet.
Next steps: @pacu will test the tool in his development server to verify the functionality of the code and point out any issues encountered with the features planned for milestone 2 if any
| pub type Result<T> = std::result::Result<T, zeckitError>; | ||
|
|
||
| #[derive(Error, Debug)] | ||
| pub enum zeckitError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please follow Rust Lang Naming conventions?
https://rust-lang.github.io/api-guidelines/naming.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright my bad would follow standard naming conventions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename README.md to follow naming conventions on Github repos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm okay would adjust this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file still here :)
| echo "📊 Current block height: ${BLOCK_COUNT}" | ||
|
|
||
| # Wait for blocks | ||
| echo "⏳ Waiting for at least 10 blocks to be mined..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why it needs to wait for blocks to be synced given that LWD should be able to wait for the node without problems when initialized
|
The thing is we need to wait for 10 blocks each for both lightwalletd and zaino Before starting |
pacu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partial review. requesting changes
|
|
||
| **Purpose:** REST API for test funds and fixtures | ||
|
|
||
| **Technology:** Python 3.11 + Flask + Gunicorn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is incorrect. The Faucet is supposed to be Rust already
| **Docker Image:** Custom Python 3.11-slim + Docker CLI | ||
|
|
||
| **Key Files:** | ||
| - `/app/app/` - Flask application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flask reference
|
|
||
| **Alternative considered:** Direct Zebra RPC → Rejected (no UA support) | ||
|
|
||
| ### Why Python Faucet? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reference to flask
| │ │ | ||
| │ ┌──────────┐ │ | ||
| │ │ Faucet │◄───────────────────────────┘ | ||
| │ │ Flask │ │ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flask reference
| **Critical Functions:** | ||
| ```python | ||
| def send_to_address(address: str, amount: float) -> dict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes reference to python faucen
| use wallet::WalletManager; | ||
|
|
||
| #[derive(Clone)] | ||
| pub struct AppState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, please check visibility needed in axum's docs and provide the least public one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this file. these should be downloaded and stored, not checked in into github
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this file. these should be downloaded not checked into github
| - Health checks | ||
| - Manual Docker Compose | ||
|
|
||
| **M2 Real Transactions:** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
M2 goals are incomplete according to the milestones presented and approved by ZCG ZcashCommunityGrants/zcashcommunitygrants#105
| - UA (ZIP-316) test vectors | ||
| - Manual Docker Compose workflow | ||
|
|
||
| ### Milestone 2: Real Transactions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this information seems repeated hence inconsistent
Milestone 2: Real Blockchain Transactions
This PR delivers M2 core functionality: a working Zcash regtest devnet with real on-chain transactions, dual backend support (Lightwalletd + Zaino), and a functional faucet API using pexpect for reliable wallet interaction.
✅ What's Delivered
Core Infrastructure:
Real Blockchain Transactions:
Developer Experience:
zecdev) with up/down/test commandsKey Technical Achievements:
📊 Test Results
Zaino Backend:
Real Transaction Proof:
🔧 Technical Highlights
Pexpect Implementation:
Healthcheck Optimization:
Backend Toggle:
📚 Documentation Added
README.md- Complete user guide with quickstart (updated)specs/technical-spec.md- 27-page implementation deep-dive (NEW)specs/architecture.md- System design and data flows (NEW)scripts/setup-mining-address.sh- Mining address configuration (NEW)🐛 Bugs Reported Upstream
🚀 What This Enables
📋 Files Changed
New Files:
specs/technical-spec.mdspecs/architecture.mdscripts/setup-mining-address.shdocker/lightwalletd/Dockerfile(Go 1.24 build)docker/lightwalletd/entrypoint.sh(healthcheck logic)Modified Files:
README.md(comprehensive rewrite)docker-compose.yml(healthcheck tuning, backend profiles)faucet/app/wallet.py(pexpect implementation)faucet/app/main.py(startup optimization)docker/configs/zebra.toml(mining configuration)✅ M2 Acceptance Criteria
From Grant:
Status:
🙏 Acknowledgments
Thanks to the Zcash community for:
Ready for review! Infrastructure is solid, real transactions working.